-
-
Notifications
You must be signed in to change notification settings - Fork 341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move methods generating completion replies to the provider #717
Move methods generating completion replies to the provider #717
Conversation
a50f81a
to
45e2e56
Compare
Hello 👋 just checking back here. It looks like all checks are passing - is there anything which needs addressing for this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krassowski This PR looks great, thank you! ❤️ Left a minor comment below.
Based on the discussion in #717, it seems that I should merge #717 in first, then you can address the merge conflicts here. Does that work best for you?
7357dec
to
c16bb54
Compare
@krassowski Thank you for the quick reply! Sorry, I meant #726, not this PR. 🤦 I'll merge #726 in first, and then approve this PR once the conflicts are addressed and CI is passing. 👍 |
@krassowski #726 is merged. |
Add an example completion provider Adjust name of variable to reflect new implementation
c16bb54
to
4dcf885
Compare
Just pinging back @dlqqq as the requested changes were made and CI is all green. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, thank you! 🎉
Thank you! |
…b#717) * Move methods generating completion replies to provider Add an example completion provider Adjust name of variable to reflect new implementation * Rename `_completion.py` → `completion_utils.py`
DefaultInlineCompletionHandler
#702This PR moves the logic for generating inline completion replies away from the
DefaultInlineCompletionHandler
and into the two new methods inBaseProvider
:generate_inline_completions
: takes a request (InlineCompletionRequest
) and returnsInlineCompletionReply
stream_inline_completions
: takes a request and yields an initiating reply (InlineCompletionReply
) withisIncomplete
set toTrue
followed by subsequent chunks (InlineCompletionStreamChunk
)This has a few advantages:
MyCompletionProvider
example (which is being included in documentation); it is also useful for manual testing without incurring costs by usage of cloud providersTo make that possible I had to move the completion models to
jupyter-ai-magics
; I believe this is ok because thePersona
model already resides in there. I kept themodels.py
from where I am re-exporting these models - we can either keep it like that or add a deprecation warning when someone imports from thejupyter_ai.completions.models
, I am fine either way.To avoid adding more methods on
BaseProvider
, I instead put the static utilities for completion methods injupyter_ai_magics/_completion.py
file; it has an underscore because I want to treat them as private implementation details so that we can change them without notice.Additionally, I fixed typing issues in
BaseInlineCompletionHandler
which crept up when the initial implementation of inline completions got reworked:.write_message()
with broad types incompatible with the types in the base class (mypy complaining) I now define.reply()
which only accepts the two models that should be allowed (completion reply and stream chunk)handle_request
andhandle_stream_request
config_manger
was renamed tojai_config_manager
because it was overridingconfig_manager
ofJupyterHandler
while having an incompatible type